Optimize move.move_file for moving files between different OSFS instances.#523
Conversation
|
@althonos Implement it as you suggested. |
|
I guess I need to lock. Otherwise the file could be deleted from src_fs during the move operation. |
|
I guess you just need to patch the |
|
Should be fine now, can I trigger the test workflow somehow? |
Since it's possible to get Windows tested in GitHub Actions as well, I'll try to update the CI to stop using AppVeyor since it's harder to maintain just for Windows. And thanks a lot @lurch for the super thorough review 👍 |
|
I fixed the failing tests with Python 2.7 that were caused by a circular import between several modules. The @lurch : any last comment before i merge? |
|
Thank you for fixing things up 👍 Looks good to me. |
| except ValueError: | ||
| # This is raised if we cannot find a common base folder. |
There was a problem hiding this comment.
Is this exception-catching still needed, now that you check if common: ?
There was a problem hiding this comment.
Yes, because frombase might raise one
|
This is looking pretty great @tfeldmann , thank you for sticking with it through all my review comments, but hopefully you agree that the extra effort was worth it 😀
🤣 Would it be much more effort to do a similar "optimized-codepath-if-both-FSes-have-a-syspath" for P.S. Hello from a fellow 🧗♂️ |
Yep, definitely improved the PR a lot and I learned a thing or two 👌 Really appreciated it!
I'll tackle this in a new PR after this one is merged
Ah, nice! 👋 |
| ) | ||
| self.assertTrue(src.exists("file.txt")) | ||
| self.assertTrue(dst.exists("target.txt")) | ||
| self.assertEqual(not dst.exists("target.txt"), cleanup) |
There was a problem hiding this comment.
Might be marginally more readable as
self.assertEqual(dst.exists("target.txt"), not cleanup)
|
Hey, any updates on this? |
|
This is in great shape, I'll be merging! Thanks again @tfeldmann :) |
4 participants
Type of changes
Checklist
Description
Using
move.move_fileto move a file between different OSFS instances is extremely slow because it first copies the file to the destination and then deletes the source.This PR adds an optimization for this use case.